Skip to content

BUILD-8956 Extract config-poetry action from build-poetry#292

Open
hedinasr wants to merge 4 commits into
masterfrom
feat/hnasr/BUILD-8956-createConfigPoetry
Open

BUILD-8956 Extract config-poetry action from build-poetry#292
hedinasr wants to merge 4 commits into
masterfrom
feat/hnasr/BUILD-8956-createConfigPoetry

Conversation

@hedinasr

Copy link
Copy Markdown
Contributor

Summary

  • Adds a new config-poetry composite action for Repox authentication, JFrog CLI setup, Poetry caching, and build number management
  • Refactors build-poetry to delegate configuration to config-poetry, mirroring the config-npm / build-npm split
  • Adds shellspec coverage for config-poetry/poetry_config.sh and updates build-poetry tests

Behavioral changes

  • build-poetry no longer fetches Artifactory reader credentials itself; that is handled by config-poetry
  • config-poetry can now be used standalone in workflows that need Poetry + Repox without a full build
  • POETRY_HTTP_BASIC_REPOX_USERNAME and POETRY_HTTP_BASIC_REPOX_PASSWORD are exported via GITHUB_ENV for downstream steps

Test plan

  • shellspec spec/config-poetry_spec.sh spec/build-poetry_spec.sh passes locally
  • CI shellspec workflow passes on the PR

@sonarqubecloud

sonarqubecloud Bot commented Jun 11, 2026

Copy link
Copy Markdown

Agentic Analysis: Early Results

Agentic Analysis and Context Augmentation are available on your project. Here are some issues that could have been prevented. Follow the links to learn how to put them into action.

3 issue(s) found across 2 file(s):

Rule File Line Message
shelldre:S1192 spec/build-poetry_spec.sh 333 Define a constant instead of using the literal 'poetry install' 5 times.
shelldre:S1192 spec/build-poetry_spec.sh 336 Define a constant instead of using the literal 'Building project...' 4 times.
shelldre:S1192 spec/config-poetry_spec.sh 125 Define a constant instead of using the literal 'version -s' 5 times.

Analyzed by SonarQube Agentic Analysis in 3.4 s

@hashicorp-vault-sonar-prod

hashicorp-vault-sonar-prod Bot commented Jun 11, 2026

Copy link
Copy Markdown

BUILD-8956

Comment thread config-poetry/action.yml Outdated
Comment thread config-poetry/action.yml
@hedinasr hedinasr marked this pull request as ready for review June 12, 2026 12:47
@hedinasr hedinasr requested a review from a team as a code owner June 12, 2026 12:47
Copilot AI review requested due to automatic review settings June 12, 2026 12:47

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR introduces a new config-poetry composite action to set up Poetry + Repox (JFrog) authentication, caching, and build-number management, and refactors build-poetry to delegate that setup to config-poetry (similar to the existing config-npm / build-npm split).

Changes:

  • Added config-poetry composite action and poetry_config.sh to configure JFrog + Poetry auth and export POETRY_HTTP_BASIC_REPOX_* via GITHUB_ENV.
  • Refactored build-poetry to stop configuring Repox for dependency install, and instead call config-poetry.
  • Added ShellSpec coverage for config-poetry/poetry_config.sh, updated build-poetry tests, and included config-poetry in kcov include patterns.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
config-poetry/action.yml New composite action to configure build number, caching, and Poetry/JFrog auth.
config-poetry/poetry_config.sh New script that configures JFrog CLI + Poetry auth and exports Repox basic-auth env vars.
config-poetry/mise.local.toml Adds mise tool/env config for installing/configuring JFrog CLI for config-poetry.
build-poetry/action.yml Delegates setup to config-poetry and adjusts outputs / vault usage accordingly.
build-poetry/build.sh Removes in-script Repox dependency setup; installs dependencies via plain poetry install.
spec/config-poetry_spec.sh New ShellSpec tests for config-poetry/poetry_config.sh.
spec/build-poetry_spec.sh Updates expectations to reflect moved Repox configuration and renamed install function.
README.md Documents the new config-poetry action and notes build-poetry now calls it.
.shellspec Adds config-poetry to the kcov include pattern.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread config-poetry/action.yml Outdated
Comment thread build-poetry/action.yml Outdated
Comment thread build-poetry/build.sh Outdated
Comment thread build-poetry/build.sh Outdated
Comment thread config-poetry/action.yml Outdated
Comment thread config-poetry/action.yml Outdated
Comment thread config-poetry/poetry_config.sh Outdated
Comment thread build-poetry/build.sh Outdated
Comment thread config-poetry/action.yml
hedinasr added 3 commits June 17, 2026 15:04
Add a standalone config-poetry composite action that fetches Vault
credentials, configures JFrog CLI and Poetry for Repox, caches Poetry
dependencies, and sets BUILD_NUMBER. Refactor build-poetry to consume
config-poetry the same way build-npm uses config-npm.
Move project version setup into config-poetry, merge setup steps, add
workflow-scoped cache keys, use global jf poetry-config, inline poetry
install in build-poetry, and split mise restore into an always() step.
@hedinasr hedinasr force-pushed the feat/hnasr/BUILD-8956-createConfigPoetry branch from 24e1672 to 0092d6a Compare June 17, 2026 13:04
Comment thread config-poetry/action.yml
Comment thread config-poetry/action.yml
@gitar-bot

gitar-bot Bot commented Jun 18, 2026

Copy link
Copy Markdown
Code Review ✅ Approved 4 resolved / 4 findings

Extracts the Poetry configuration logic into a standalone composite action, enabling modular reuse while addressing previous issues with mise environment restoration and unused input declarations.

✅ 4 resolved
Bug: mise restore runs in wrong dir when working-directory != '.'

📄 config-poetry/action.yml:47-61 📄 config-poetry/action.yml:117-131
In config-poetry/action.yml, the mise.local.toml file and the MISE_BACKUP temp dir are created by the "Set local action paths" step (lines 47-77), which has NO working-directory, so they live in the GitHub workspace root. But the "Configure Poetry authentication" step (line 120) sets working-directory: ${{ inputs.working-directory }} and then runs the restore logic:

rm mise.local.toml
mv "$MISE_BACKUP"/* ... ./ ...

When working-directory is not ., this runs inside the subdirectory:

  • rm mise.local.toml fails because the file is in the workspace root, not the subdir. GitHub Actions runs bash steps with -e, so the step fails and breaks the build.
  • Even if it didn't fail, the user's original mise files would be restored into the subdirectory (./) instead of the workspace root, leaving the root in an inconsistent state with config-poetry's mise.local.toml never removed.

Note this diverges from config-npm/action.yml, whose "Configure NPM authentication" step (line 101-114) does the same backup/restore but does NOT set working-directory, keeping creation and restore in the same (root) directory. build-poetry exposes a working-directory input and passes it through, so any consumer using a non-root working directory will hit this regression (previously build-poetry never rm'd the file).

Fix: keep working-directory only for the poetry_config.sh invocation (which needs the project dir) and perform the mise restore relative to the workspace root, or split the restore into a separate step without working-directory (matching config-npm).

Bug: mise files not restored if a step fails before restore

📄 config-poetry/action.yml:72-77 📄 config-poetry/action.yml:117-131
In config-poetry/action.yml, the "Set local action paths" step moves the user's real mise files into a temp backup and substitutes config-poetry's mise.local.toml. The restore happens inside the "Configure Poetry authentication" step, which is NOT marked always() and only runs after several intermediate steps (get-build-number, cache, mise-action, vault). If any of those steps fails, the restore never runs: the user's mise files remain stranded in the temp directory and config-poetry's mise.local.toml is left in place. For standalone config-poetry usage, this leaves the checkout in an inconsistent state for any downstream steps. Consider performing the restore in a dedicated if: always() step (see the suggested fix for the working-directory finding, which also addresses this).

Quality: config-poetry input poetry-virtualenvs-path is declared but never used

📄 config-poetry/action.yml:18-21 📄 build-poetry/action.yml:119
config-poetry/action.yml declares and documents the poetry-virtualenvs-path input (lines 18-21, and in README), and build-poetry/action.yml passes it to the action (line 119), but the value is never referenced in any of the config-poetry steps. Unlike build-poetry, which sets POETRY_VIRTUALENVS_PATH as an env var for build.sh, config-poetry neither exports it to $GITHUB_ENV nor uses it for the cache path.

Impact: when config-poetry is used standalone (the new supported use case), a caller who sets poetry-virtualenvs-path will have it silently ignored; Poetry will use its default virtualenv location, which may fall outside the cached poetry-cache-dir, so virtualenvs won't be cached/restored as the docs imply.

Suggested fix: either export POETRY_VIRTUALENVS_PATH to $GITHUB_ENV in config-poetry so downstream poetry install honors it (consistent with the input's documented purpose), or remove the unused input and the corresponding README/build-poetry wiring to avoid implying behavior that doesn't exist.

Edge Case: Cache key sanitization only replaces spaces, not other illegal chars

📄 config-poetry/action.yml:96-110
The new sanitize_workflow step sanitizes the workflow name for the cache key with workflow_name=${WORKFLOW_NAME// /-}, replacing only spaces. GitHub Actions cache keys cannot contain commas (and github.workflow falls back to the workflow file path when no name: is set, which can contain / and other characters). A workflow whose name contains a comma would produce an invalid cache key and cause the Cache local Poetry cache step to fail. Consider sanitizing more broadly, e.g. replacing any character outside an allowlist:

workflow_name=$(echo "$WORKFLOW_NAME" | tr -c '[:alnum:]._-' '-')

This is a low-probability edge case (most workflow names are simple), hence minor, but worth hardening since the key now feeds a required cache step.

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.

Comment with these commands to change:

Auto-apply Compact
gitar auto-apply:on         
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Gitar

@sonarqubecloud

Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants